Conversation
|
Hey @AuroraTea thanks for raising this! Would you mind sharing more details about how you'd want to use this? In the library's core use-case we were expecting it to be used in HTTP services, where a panic wouldn't be ideal, but if there are cases you'd be looking to use this that'd make sense, then sure. I get the point that (hopefully) a consumer would be doing the |
|
This PR makes sense to me to have a Using |
|
@sonasingh46 Thank you! Mostly. @jamietanna Sorry I'm busy recently. Mostly like sonasingh46's Example. //IsNull and IsSpecified handling
f1, _ := req.field1.Get()
f2, _ := req.field2.Get()
f3, _ := req.field3.Get()
sql, args, _ := sqpg.
Select("*").
From("products").
Where(sq.Eq{"id": f1}).
Where(sq.Eq{"name": f2}).
Where(sq.Eq{"supplier": f3}).
ToSql()vs //IsNull and IsSpecified handling
sql, args, _ := sqpg.
Select("*").
From("products").
Where(sq.Eq{"id": req.field1.MustGet()}).
Where(sq.Eq{"name": req.field2.MustGet()}).
Where(sq.Eq{"supplier": req.field3.MustGet()}).
ToSql()No additional variable (Especially additional variable names), No additional logic.
Just because it's used in HTTP services I'm going to assume that the user should have judged it before calling: the backend needs to validate, and those who don't need to check for IsNull and IsSpecified would not choose the nullable approach. Also adding |
|
@AuroraTea I can't seem to push back to your fork, would you mind un-protecting your I've got the following changes to add: diff --git nullable_example_test.go nullable_example_test.go
index bb0394c..c391817 100644
--- nullable_example_test.go
+++ nullable_example_test.go
@@ -252,6 +252,7 @@ func ExampleNullable_unmarshalRequired() {
return
}
fmt.Printf("obj.Name.Get(): %#v <nil>\n", val)
+ fmt.Printf("obj.Name.MustGet(): %#v\n", obj.Name.MustGet())
fmt.Println("---")
// when it's set explicitly to a specific value
@@ -274,6 +275,7 @@ func ExampleNullable_unmarshalRequired() {
return
}
fmt.Printf("obj.Name.Get(): %#v <nil>\n", val)
+ fmt.Printf("obj.Name.MustGet(): %#v\n", obj.Name.MustGet())
fmt.Println("---")
// Output:
@@ -289,11 +291,13 @@ func ExampleNullable_unmarshalRequired() {
// obj.Name.IsSpecified(): true
// obj.Name.IsNull(): false
// obj.Name.Get(): "" <nil>
+ // obj.Name.MustGet(): ""
// ---
// Value:
// obj.Name.IsSpecified(): true
// obj.Name.IsNull(): false
// obj.Name.Get(): "foo" <nil>
+ // obj.Name.MustGet(): "foo"
// ---
}And then a tweak on the commit message (which I'm happy to do at time of merge) Details |
|
Happy to merge afterwards, appreciate you raising this 🚀 |
|
@jamietanna For commit message, I have now removed the protection rules. Give it a try, and if it doesn't work, I'll attempt re-raising from a new branch. Surprisingly it's possible to change the commit message of a commit already submitted to GitHub. When you have time, could you please teach me how to do it? |
|
Hey @AuroraTea sorry it wasn't clear in my diff - I can see that I'm happy for you to amend. Sure! The quickest way to do so is: # assuming you're on your branch
# edit the files
git add -p
git commit --amend
# write your new commit message
# git push -fThat's it 🚀 Note that force pushing can be dangerous and destructive, so avoid doing it on branches where other folks may be contributing / using (i.e. |
In the cases where a user has explicitly checked `IsSpecified()` and `!IsNull()`, calling `Get` only to discard the `err` can be a bit awkward, so instead we can introduce a `MustGet` method to retrieve the value and panic if there is an error, as is common to do with Go libraries.
|
I used to have multiple commits but use squash or rebase for merging PR. |
Reason
Many cases will do
IsSpecified()andisNull()judgment before callingGet().Compared to
val, _ := foo.Get(),foo.MustGet()in many scenarios will be more atomic, the call logic does not have to truncate or be separated from other logic (such as assigning a value to another struct).Test